fix(relay): align NIP-11 max_limit with REQ ceiling - #3635
Open
wpfleger96 wants to merge 3 commits into
Open
Conversation
…rces NIP-11 advertised `max_limit: 10_000`, but the effective websocket page ceiling was 1,000: the REQ path never sets `EventQuery::max_limit`, so `query_events` applied its `unwrap_or(1000)` clamp to every historical query. A client trusting the advertisement asks for 10,000, silently receives 1,000, and reads that short page as exhaustion — dropping up to 9,000 events with no error. `MAX_HISTORICAL_LIMIT = 2_000` in the REQ path was dead for the same reason: nothing could survive the DB clamp. Both call sites now use the DB clamp default directly, so one constant is the single source of truth for the advertised ceiling and the enforced one. No behavior change — 1,000 was already the real limit on every path. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The search path clamped its emission target to the advertised ceiling, but how far it could scan to reach that target was bounded separately by a bare 10-page loop over 100-hit pages. The product happened to equal the ceiling, so raising the ceiling — or shrinking a page — would leave search emitting short pages while still advertising the larger number: the same silent under-delivery the ceiling exists to prevent. The page count is now ceiling-divided from the shared limit over a named page size, so scan capacity tracks the advertisement by construction, and a guard test asserts the relation holds with no spare page of slack. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The scan-budget comments and test framing claimed the derivation lets search reach the advertised limit. It cannot: the budget bounds FTS candidates scanned, and post-filtering (NIP-01 match, channel access, reader visibility, dedup) discards an unpredictable share before emission. NIP-11 defines max_limit as a clamp on the request, not a promised response count, so a short search result was never a conformance violation. The derivation stays — it is a resource policy that tracks the ceiling — but its justification no longer overreaches. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Buzz's NIP-11 document advertised
limitation.max_limit: 10_000, but the effective websocket REQ page ceiling was1_000— a 10x lie.The websocket REQ path never sets
EventQuery::max_limit, soquery_eventsapplied its ownunwrap_or(1000)clamp to every historical query. Only the COUNT fallback (apply_count_fallback_limit) ever raises that clamp. A client that trusts the advertised value asks for 10,000 events, silently receives 1,000, and — with no error and no continuation signal — reads that short page as exhaustion. Up to 9,000 events are dropped without anyone noticing.MAX_HISTORICAL_LIMIT = 2_000inhandlers/req.rswas dead weight for the same reason: nothing clamped to 2,000 could survive the DB's 1,000 clamp one layer down.Change
buzz_db::DEFAULT_MAX_PAGE_LIMIT(1_000) is now the single source of truth. It is thequery_eventsclamp default, the value both REQ clamp sites use, and the value advertised as NIP-11max_limit.MAX_HISTORICAL_LIMITis removed rather than re-pointed — an alias for a constant used four lines away adds a name without adding meaning.The NIP-50 search path carries a second, independent bound. It clamps its emission target to the shared ceiling like any other REQ, but how many FTS candidates it will scan was bounded separately, by a bare 10-page loop over 100-hit pages. That product only coincidentally equalled the ceiling, so raising the ceiling — or shrinking a page — would shrink the scan relative to what clients may now request, degrading search quality while nothing in the code registered the change. The page count is now ceiling-divided from
DEFAULT_MAX_PAGE_LIMITover a namedSEARCH_PAGE_SIZE, so the scan budget tracks the advertised ceiling by construction.That budget is a resource policy, not a delivery promise. It bounds candidates scanned, not events emitted: post-filtering (NIP-01 match, channel access, reader visibility, dedup) discards an unpredictable share of every page, so a search result smaller than the requested limit remains possible. This is not a NIP-11 violation —
max_limitis defined as a clamp the relay applies to a requestedlimit, not a guaranteed count in the response.Two guards hold the pair together:
req_filter_limit_clamps_to_advertised_nip11_max_limitreadsmax_limitback out of a builtRelayInfoand asserts the REQ path clamps to exactly that number.search_scan_capacity_covers_advertised_nip11_max_limitasserts the scan budget covers exactly one advertised ceiling's worth of candidates — no less, and with no spare page of slack, so the derivation can't be quietly replaced by a hand-tuned constant that happens to pass today.Behavior
Websocket behavior is unchanged: 1,000 was already the real ceiling on every path, including NIP-50. The advertisement now tells the truth about it. Raising the effective limit is a capacity decision and is deliberately not made here.
The generic HTTP bridge's page-2+ offsets do change, as a consequence of the corrected clamp.
extract_page_offsetsizes a page fromquery.limitbefore the DB clamp applies, so an absent limit previously produced an offset of 2,000 and a requested 1,500 produced 1,500 — while the page actually returned held at most 1,000 rows. Both now produce 1,000. This corrects paging that had been skipping rows the previous page never returned;extract_page_offset_sizes_pages_from_clamped_limitlocks it down.Scope note
The bridge's per-endpoint ceilings —
BRIDGE_WINDOW_MAX_LIMIT(200) for channel windows andBRIDGE_THREAD_MAX_LIMIT(500) for thread reads — are endpoint contracts on a non-NIP-01 transport, not values NIP-11 speaks for, and are unchanged.